The separator a string field is escaped against is the one it is joined with - #499
Conversation
A string field that contained the separator split its own field in two and
shifted every field after it. Serialized on master, a mate named
"Joyeux Mouton" produced
sc_p 1 333 0 15 ... 0 Joyeux Mouton 0
where the client reads a name and then a stray field.
The escaping was already there - StringSerializer replaces the separator with
"^" for any field that is not the last one - but the separator it was handed is
PacketIndexAttribute.SpecialSeparator, which an ordinary property never
declares. IsNullOrEmpty then short-circuited and the value went out untouched,
while the real " " separator was applied afterwards by PacketSerializer and
never reached the substitution. So the guard only ever fired for the handful of
properties that declare a separator of their own.
StringSerializer now takes the separator the field is actually joined with. The
last field still keeps its spaces, which chat lines and miniland intros rely on,
and a property with a SpecialSeparator still escapes that one rather than the
space.
ScnPacket.Name has carried "Spaces should be replaced by ^" as a doc comment
this whole time with nothing enforcing it.
The 115 existing tests pin a lot of exact wire output and all still pass.
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Walkthrough
ChangesField separator escaping
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR corrects delimiter escaping for non-final string fields while preserving final-field and custom-separator behavior, with focused tests covering the change. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Two gaps left by escaping against the field separator alone.
A string inside a sub-packet only escaped its own separator. Its value sits in
a field of the packet above it, so a space still split that outer field:
pinit 0 0|0|1|10|Joyeux Mouton|0|0|0|0|0|0
Nested strings now escape the outer separator as well.
The last field of a packet keeps its spaces, which is what chat lines need, but
a few fields carry text the client expects "^"-encoded wherever they sit -
mlintro, the miniland message on mlinfobr, the family message on ginfo. They
declare EscapeSpaces rather than every caller remembering to substitute.
122 tests pass.
1 participant
Serializer-side fix for the class of bug NosCoreIO/NosCore#2336 works around in a caller.
The defect
A string field containing the field separator splits its own field in two and shifts every field after it. On master, a mate named
Joyeux Mouton:The escaping already existed — aimed at the wrong separator
StringSerializerreplaces the separator with^for any field that is not the last one. But the separator it is handed isPacketIndexAttribute.SpecialSeparator, which an ordinary property never declares, soIsNullOrEmptyshort-circuits and the value goes out untouched. The real" "is applied afterwards byPacketSerializerand never reaches the substitution.Measured on master with throwaway packets:
probe 1 Joyeux Moutonprobeb 1 Joyeux Mouton 2SpecialSeparator = " "probec 1 Joyeux^Mouton 2So the guard only ever fired for the handful of properties declaring a separator of their own — which is why
SpecialSeparator = " ""fixes" it and why the whole thing looked like it worked.ScnPacket.Namehas carried/// Spaces should be replaced by "^"this whole time with nothing enforcing it.The change
StringSerializertakes the separator the field is actually joined with, hoisting the existingsplitterabove thePropertySerializercall so it can be passed down. Deliberately unchanged:SpecialSeparatorstill escapes that one, not the space""on that path, so this is not a blanket changeTests
5 new. Two of them fail on master and pass here; the other three pin the behaviour that must not change (last field,
SpecialSeparator) and pass either way — verified by stashing the fix and re-running.Coverage includes the three real packets that carry a mate name —
ScpPacket(index 31),ScnPacket(36),InPacket(1) — all non-last, all now escaped.The 115 existing tests pin a lot of exact wire output. All still pass: 120/120.
Downstream
Once this ships,
MateExtensions.DisplayNamein NosCoreIO/NosCore#2336 becomes redundant.The two manual escapes in NosCore —
MinilandEntranceHandlerandMlobjPacketHandler— cannot be removed.MsgPacket.MessageandMlintroPacket.Introare both last-index fields, where the serializer deliberately does not escape; those are encoding the space for the client's own display, which is a different concern from field splitting.Summary by CodeRabbit